Remove async std dependency by praveenperera · Pull Request #103 · bitcoindevkit/rust-esplora-client

praveenperera · 2024-10-13T20:50:37Z

Closes: #102

praveenperera · 2024-10-14T00:26:38Z

can't seem to get this working with rust 1.63.0

when i run cargo update -p tokio-util --precise "0.7.11"

I get: error: package ID specification tokio-util did not match any packages

tnull · 2024-10-14T07:40:58Z

+#[cfg(feature = "async-std")]
+pub use async_std::task::sleep;
+
+#[cfg(not(any(feature = "tokio", feature = "async-std")))]


Mhh, in general features should be additive, so I'm not sure we want to add a layer of non-additive features here. Also the choice between tokio and async-std is pretty limited, and given the former is the defacto standard by now we may as well drop async-std entirely, IMO. I also think we should introduce a Sleeper trait an provide a tokio-based default implementation, allowing users to implement it any other way too, instead of only allowing these two choices.

E.g., I think WASM users won't be able to use tokio, and it looks like they won't be able to use async-std either (cf. async-rs/async-std#220). So, this change would really won't leave a way for them to keep using the esplora client, IIUC. (Although that issue is now kind of pre-existing since the async-std dependency has been introduced.

I’m good with only supporting Tokio.

I think WASM users would just use the blocking client and skip the async features all together?

Adding tokio or async-std would only be needed is the async features is also enabled.

I think WASM users would just use the blocking client and skip the async features all together?

I don't think they can use the blocking client. Usually WASM is async-everything but it's driven by WASM's own executor. So AFAIK we need to allow for a way to bring your own runtime to let them continue to use the rust-esplora-client crate.

Interesting, i didn't know that, i've only used WASM for any network calls just number crunching. Does their runtime have a task too? Could we use that with features and conditional compilation?

Interesting, i didn't know that, i've only used WASM for any network calls just number crunching. Does their runtime have a task too?

Mhh, I'm not sure, I think most of them are just using wasm_bindgen and did so successfully previously with rust-esplora-client and projects based on it. AFAIK just using async reqwest works decently.

Could we use that with features and conditional compilation?

I really don't think we should go there and introduce even more assumptions on how the user wants to use the library, which is essentially a a bunch of types, i.e., a thin wrapper around an HTTP client. I think the API of this crate should be flexible but overall kept very very simple. Tbh, I don't see a reason why this crate should have much more than a single dependency on serde_json, especially when we end up going the #97 direction.

I think that makes sense, as long as we keep the logic for retires and that kinda stuff.

I agree with tnull's comments, I think it's pretty straightforward to have and use a Sleeper trait, using the tokio (already existing one) as the default batteries included implementation, alongside allowing the user to implement their own, for example, gloo-timers for WASM.

praveenperera · 2024-10-15T16:59:33Z

@tnull @oleonardolima using a trait instead now

oleonardolima · 2024-10-15T17:56:13Z

@tnull @oleonardolima using a trait instead now

Awesome! I'd still not bring async-std, and have only tokio as the default one, the user could use one of their choice and pass it when building the clients.

praveenperera · 2024-10-15T18:02:51Z

@oleonardolima I could get rid of it, but its an optional dep and I thought it would be nice keep in there because if it's zero cost if you don't use it and if you are using async-std you can still use it as:

Builder::new(...)

Instead of

struct AsyncStd;

impl Runtime for AsyncStd {...}

Builder::<AsyncStd>::new_custom_runtime(....)

If you don't think its worth it tho, I'll get rid of it.

tnull · 2024-10-15T18:21:34Z

@tnull @oleonardolima using a trait instead now

Awesome! I'd still not bring async-std, and have only tokio as the default one, the user could use one of their choice and pass it when building the clients.

Also +1 for dropping async-std as tokio is the (for better or worse) the most commonly used runtime.

praveenperera · 2024-10-15T18:36:42Z

Alright guys you convinced me, I never use async-std or see it much anymore, I removed it completely.

oleonardolima

I guess there are some old commits that could be dropped, or everything squashed into a single one.

I'm still unsure about the appropriate feature structure, looking forward to have other's opinions about it.

oleonardolima · 2024-10-15T20:48:30Z

 blocking-https-bundled = ["blocking", "minreq/https-bundled"]
-async = ["async-std", "reqwest", "reqwest/socks"]
+
+tokio = ["dep:tokio", "async"]


I wonder if we do need to have tokio as a separate feature, maybe we can have as part of the async one, as we did for async-std, still unsure about it. WDYT ?

I think it makes sense, its a default feature, but if a user wanted to use a different async runtime, but still wanted to use reqwest, without tokio like in WASM, they could disable default features and add wasm and async, features.

I think it gives more flexibilty without adding annoyance since its a default feature.

Yes, I was afraid of bringing some other unwanted dependencies through tokio, but reqwest already uses it with features net and time, so it should be unified, looks fine.

oleonardolima

It seems to be failing regarding using only blocking feature, I left some comments/suggestions.

coveralls · 2024-10-17T19:11:42Z

Pull Request Test Coverage Report for Build 11880146166

Details

8 of 16 (50.0%) changed or added relevant lines in 2 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.4%) to 88.426%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/lib.rs	7	10	70.0%
src/async.rs	1	6	16.67%

Files with Coverage Reduction	New Missed Lines	%
src/async.rs	1	80.0%

Totals
Change from base Build 11879843924:	-0.4%
Covered Lines:	1146
Relevant Lines:	1296

💛 - Coveralls

oleonardolima · 2024-11-08T00:59:56Z

Im not sure, but I would use refactor(retry)!: ..., as I consider it a breaking change on how the feature works.

ValuedMammal · 2024-11-12T00:05:23Z

I think we will need #110 to get this to run in CI.

ValuedMammal · 2024-11-13T04:48:06Z

@praveenperera Can you rebase?

praveenperera · 2024-11-13T15:37:41Z

@ValuedMammal done

ValuedMammal

ACK 9963f27

ValuedMammal · 2024-11-13T17:52:58Z

Code review ACK

It could use some documentation improvements, but that can be done in a follow up PR. I tested upgrading bdk_esplora to use this branch and it seems to work. For the record I did not try using the async client with a runtime other than tokio, but I'm interested in confirming if this change will be suitable to use for example in a WASM environment.

MaxFangX · 2024-11-15T06:16:01Z

I can confirm that this patch compiles and works in our downstream code after reducing the tokio requirement from 1.38->1.36.

oleonardolima

utACK 9963f27

oleonardolima · 2024-11-15T22:56:14Z

+#[cfg(feature = "async")]
+use r#async::Sleeper;


nit: we wouldn't need this other import if we use the full r#async::Sleeper below instead. Could be addressed in a future PR.

ValuedMammal

reACK 27c4125

oleonardolima

tACK 27c4125

Thanks for working on this one!

It looks pretty straightforward right now, and I was able to run it as examples with custom async-std and gloo-timers, instead of the default tokio runtime while only relying on the async-https feature.

tnull

LGTM

90fd1a2 docs(esplora): update README.md (valued mammal) f0e6395 deps(esplora): bump `esplora-client` to 0.11.0 (valued mammal) Pull request description: Update `bdk_esplora` to depend on esplora-client 0.11.0 ### Notes to the reviewers bitcoindevkit/rust-esplora-client#103 added a generic type parameter to `AsyncClient` representing a user-defined `Sleeper` and that change is reflected here in order to call the underlying API methods. We also add a new build feature "tokio" that enables the corresponding feature in rust-esplora-client. closes #1742 ### Changelog notice `bdk_esplora`: Bump `esplora-client` to 0.11.0 ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing * [x] This pull request breaks the existing API * [x] I'm linking the issue being fixed by this PR ACKs for top commit: notmandatory: tACK 90fd1a2 oleonardolima: ACK 90fd1a2 Tree-SHA512: 98d529d3bb0dbbf4bbafeea7d30ec5e5816ac9800ba2cb7981fc6189b4b02774956c3ddbb74bf0b3e6e22798bfced36508263e4e89c248b7a6240c5c7109107b

27c41258cfa10b5a88598364a6605b8502f79a8e refactor(async)!: remove async-std dependency, allow custom runtime (Praveen Perera) Pull request description: Closes: #102 ACKs for top commit: ValuedMammal: reACK 27c41258cfa10b5a88598364a6605b8502f79a8e oleonardolima: tACK 27c41258cfa10b5a88598364a6605b8502f79a8e Tree-SHA512: c25e9ca65441b2a53c3eabe50426c4f223566e024704db36929797c86be48899df7e7a7de75231cab8c1c70ac87d7a8bd029cdc466b8b3e47122db4ee963d393

praveenperera force-pushed the remove-async-std-dep branch from 6718f1c to fdc7aed Compare October 13, 2024 21:22

praveenperera marked this pull request as draft October 13, 2024 21:28

praveenperera marked this pull request as ready for review October 14, 2024 00:28

tnull reviewed Oct 14, 2024

View reviewed changes

praveenperera requested review from oleonardolima and tnull October 15, 2024 16:59

praveenperera force-pushed the remove-async-std-dep branch 5 times, most recently from f6695b3 to 35b6377 Compare October 15, 2024 17:50

oleonardolima reviewed Oct 15, 2024

View reviewed changes

praveenperera requested a review from oleonardolima October 15, 2024 21:16

oleonardolima reviewed Oct 17, 2024

View reviewed changes

Comment thread src/lib.rs Outdated

Comment thread src/lib.rs Outdated

Comment thread src/lib.rs Outdated

Comment thread src/lib.rs Outdated

Comment thread src/lib.rs Outdated

oleonardolima assigned praveenperera Oct 17, 2024

oleonardolima added the enhancement New feature or request label Oct 17, 2024

oleonardolima requested review from ValuedMammal and notmandatory and removed request for tnull October 17, 2024 18:52

praveenperera force-pushed the remove-async-std-dep branch from 6c26120 to d248d90 Compare October 17, 2024 18:55

ValuedMammal reviewed Oct 21, 2024

View reviewed changes

Comment thread Cargo.toml Outdated

Comment thread src/lib.rs Outdated

Comment thread src/sleeper.rs Outdated

Comment thread src/sleeper.rs Outdated

ValuedMammal reviewed Oct 23, 2024

View reviewed changes

Comment thread src/lib.rs Outdated

Comment thread src/lib.rs Outdated

praveenperera commented Oct 28, 2024

View reviewed changes

Comment thread src/lib.rs Outdated

praveenperera force-pushed the remove-async-std-dep branch from 01fd667 to 7cae4b8 Compare November 10, 2024 16:06

praveenperera requested a review from oleonardolima November 10, 2024 16:07

praveenperera force-pushed the remove-async-std-dep branch 2 times, most recently from 7cae4b8 to 9963f27 Compare November 13, 2024 15:37

ValuedMammal approved these changes Nov 13, 2024

View reviewed changes

MaxFangX reviewed Nov 15, 2024

View reviewed changes

Comment thread Cargo.toml Outdated

This was referenced Nov 15, 2024

refactor(async)!: add Sleeper trait to allow custom runtime #107

Closed

Add get_address_stats and get_address_txns endpoints #104

Merged

oleonardolima reviewed Nov 15, 2024

View reviewed changes

oleonardolima reviewed Nov 16, 2024

View reviewed changes

Comment thread src/lib.rs Outdated

praveenperera force-pushed the remove-async-std-dep branch 2 times, most recently from 6346012 to a5aa7f4 Compare November 17, 2024 15:37

refactor(async)!: remove async-std dependency, allow custom runtime

27c4125

praveenperera force-pushed the remove-async-std-dep branch from a5aa7f4 to 27c4125 Compare November 17, 2024 15:40

ValuedMammal approved these changes Nov 17, 2024

View reviewed changes

oleonardolima approved these changes Nov 18, 2024

View reviewed changes

tnull approved these changes Nov 18, 2024

View reviewed changes

oleonardolima mentioned this pull request Nov 19, 2024

Retry mechanism broken on WASM bitcoindevkit/bdk#1683

Closed

ValuedMammal merged commit 9f4b3b9 into bitcoindevkit:master Nov 19, 2024

praveenperera deleted the remove-async-std-dep branch November 19, 2024 18:40

notmandatory mentioned this pull request Nov 21, 2024

Bump electrum-client and esplora-client versions bitcoindevkit/bdk#1742

Closed

7 tasks

ValuedMammal mentioned this pull request Dec 1, 2024

deps(esplora): bump esplora-client to 0.11.0 bitcoindevkit/bdk#1746

Merged

5 tasks

Conversation

praveenperera commented Oct 13, 2024

Uh oh!

praveenperera commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

praveenperera commented Oct 15, 2024

Uh oh!

oleonardolima commented Oct 15, 2024

Uh oh!

praveenperera commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull commented Oct 15, 2024

Uh oh!

praveenperera commented Oct 15, 2024

Uh oh!

oleonardolima left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oleonardolima left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coveralls commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 11880146166

Details

💛 - Coveralls

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oleonardolima commented Nov 8, 2024

Uh oh!

ValuedMammal commented Nov 12, 2024

Uh oh!

ValuedMammal commented Nov 13, 2024

Uh oh!

praveenperera commented Nov 13, 2024

Uh oh!

ValuedMammal left a comment

Choose a reason for hiding this comment

Uh oh!

ValuedMammal commented Nov 13, 2024

praveenperera commented Oct 14, 2024 •

edited

Loading

tnull Oct 14, 2024 •

edited

Loading

praveenperera commented Oct 15, 2024 •

edited

Loading

coveralls commented Oct 17, 2024 •

edited

Loading